hi @agilgur5 , artifacts gc will only be running when task reconciliation is completed. I think the pod has finished running at this time. and i think this is a normal operation to delete the workflow, no need to force, increase the cost of the deletion operation, because this is not a gc problem that requires forced deletion

TaskResult reconciliation should already be completed. In your issue, I'm pretty sure the ArtifactGC Pod itself fails (due to the architecture). As it failed, the Controller cannot be sure that the artifact was deleted or not

I'm pretty sure the ArtifactGC Pod itself fails (due to the architecture)

Hmm, no, ArtifactGC can run on arm64; your Pod that fails due to arch is the argosay image's arch -- see #11613 (comment) .

artifacts gc will only be running when task reconciliation is completed

TaskResult reconciliation should already be completed

Or you're experiencing a different race condition from v3.5.3+ / v3.5.5+. This actually sounds like the root cause is #12993, which was recently fixed in #13454

I'm pretty sure the ArtifactGC Pod itself fails (due to the architecture)

Hmm, no, ArtifactGC can run on arm64; your Pod that fails due to arch is the argosay image's arch -- see #11613 (comment) that I was going to fix recently as I may have permissions to the image repositories.

Yes, I know that the reason why I failed to run is because the image argoproj/argosay:v2 in the examples does not support the linux/arm64 architecture, not because artifact-gc cannot run in arm64. I raised this PR because my workflow failed and no artifact was generated, so there is no need to do GC. I hope to be able to delete the workflow directly without adding additional operations force

I raised this PR because my workflow failed and no artifact was generated, so there is no need to do GC.

Yes, the second half of my previous comment is relevant with regard to TaskResult reconciliation. This looks like it is caused by a bug in TaskResult reconciliation and not specific to ArtifactGC

thanks @agilgur5 , i got it

Ok this makes more sense now given your last comment on the issue, thanks again for elaborating!

From my first comment above though:

A length of 0 could also mean that an ArtifactGC Pod has yet to run

I think we could potentially remove this variable entirely and just replace with a check that all ArtifactGC Pods were recouped?

Otherwise it looks like anyPodSuccess is only used for the one finalizer removal check you linked to, so perhaps want to invert that or rename the variable if it's still needed.

cc @juliev0 here on the PR as well

Thanks for finding this.

So, I commented in the issue here, but will add here as well:

Looking at the anyPodSuccess code, I'm pretty sure it was probably just for the purpose of eliminating unnecessary work, to avoid doing the woc.allArtifactsDeleted() check if none of the Artifact GC Pods had even finished and succeeded yet. We can remove it if we need to. I'm looking at the logic for woc.allArtifactsDeleted() and it actually doesn't look that expensive , so I'd be okay to remove the anyPodSuccess altogether if it makes the code simpler.

I'm surprised we haven't encountered this issue before, of a Workflow that creates no artifacts.

thanks for your patience @agilgur5 @juliev0 , In this PR I removed anyPodSuccess, allArtifactsDeleted is enough to ensure that artifacts can be GCed

thanks for the modification! The e2e tests are sometimes flakey, so if the e2e test failures are unrelated to your changes, feel free to push empty commits until it passes

so I'd be okay to remove the anyPodSuccess altogether if it makes the code simpler.

@juliev0 did you see my comment above? I thought the check might be to make sure that there were no leftover ArtifactGC Pods that were not yet recouped. But if it's even simpler than that, that works

feel free to push empty commits until it passes

Given that we have 2 approvers here, it'd be more efficient for one of us to just rerun the unsuccessful jobs (and any member can do /retest now)

I'm surprised we haven't encountered this issue before, of a Workflow that creates no artifacts.

It's very possible people have been using forceFinalizerRemoval to workaround this. Although I also just think there aren't that many ArtifactGC users yet.

Speaking of, it'd be nice to add an E2E test for this

Ok, I will add an e2e test

I thought the check might be to make sure that there were no leftover ArtifactGC Pods that were not yet recouped.

Hmm, does it accomplish that? I'm having trouble following if so. I think it was just to eliminate work.

Thanks for adding the test @chengjoey. If I'm not mistaken, I think you should be able to add it as another case to this test. This test verifies that the finalizer is removed by checking that the workflow actually gets deleted.

mostly consistency comments on the test below

thanks @agilgur5 , all has done

Yes, thank you @chengjoey !